Skip to content

In STRICT mode, only define assert when ASSERTIONS is set #20592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 2, 2023
Merged

In STRICT mode, only define assert when ASSERTIONS is set #20592

merged 1 commit into from
Nov 2, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 1, 2023 •
edited
Loading

This matches the behaviour of MINIMAL_RUNTIME, and supports the practice
of not including asserts in release builds.

Also remove the closure externs for assert so that closure can
correctly error out end assert is not defined.

Helps with #20504

@sbc100 sbc100 requested a review from kripken November 1, 2023 19:42
@sbc100 sbc100 force-pushed the no_define_assert branch 2 times, most recently from 75aaf0b to ad52571 Compare November 1, 2023 20:05
kripken
kripken reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not have any assertions outside of ifdef ASSERTIONS? That is, ones we want to be checked even in release builds?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 1, 2023

Do we not have any assertions outside of ifdef ASSERTIONS? That is, ones we want to be checked even in release builds?

Not that I know of.

If we do, perhaps we should give it a different name and make is as small as possible.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 1, 2023

Do we not have any assertions outside of ifdef ASSERTIONS? That is, ones we want to be checked even in release builds?

Not that I know of.

If we do, perhaps we should give it a different name and make is as small as possible.

It looks like that tests found at least one instance.. and we have one more being fixes in: #20504

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 1, 2023

Do we not have any assertions outside of ifdef ASSERTIONS? That is, ones we want to be checked even in release builds?

Also, if we did have such code it would already not be usable under MINIMAL_RUNTIME.

@kripken
Copy link
Member

kripken commented Nov 1, 2023

I see, this makes sense to me then.

I wonder if we can minimize the risk to users, though? Atm, if user JS code uses assert without guards they'll hit this issue, for example. Normally with such changes we add extra checks in assertions mode to catch that, but here that would be circular 😆 so I'm not sure how best to do this.

One option is to leave assert as it is, and convert all of our existing uses of it to a new function which does not exist in release builds. While converting them, we'd also be auditing the code to avoid missing ifdefs. Do you think that would be worth it?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 1, 2023

I see, this makes sense to me then.

I wonder if we can minimize the risk to users, though? Atm, if user JS code uses assert without guards they'll hit this issue, for example. Normally with such changes we add extra checks in assertions mode to catch that, but here that would be circular 😆 so I'm not sure how best to do this.

But this only effects users who have opt'd into STRICT mode, and that set of users will likely know to look at the ChangeLog if they see and error like this on upgrade.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 1, 2023

I should add that one of my motivations to landing this is to increase parity with MINIMAL_RUNTIME.. we need to ensure that our internal assert calls are all protected by if ASSERTIONS and this allows me to test that without having to make all our tests fully MINIMAL_RUNTIME compatible.

@kripken
Copy link
Member

kripken commented Nov 2, 2023

Ah, you're right. For STRICT mode this definitely makes sense.

kripken
kripken reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comments

@sbc100
In STRICT mode, only define assert when ASSERTIONS is set
31e6d0a 
This matches the behaviour of MINIMAL_RUNTIME, and supports the practice
of not including asserts in release builds.

Also remove the closure externs for `assert` so that closure can
correctly error out end `assert` is not defined.

Helps with emscripten-core#20504
@sbc100 sbc100 merged commit a2f3b92 into emscripten-core:main Nov 2, 2023
@sbc100 sbc100 deleted the no_define_assert branch November 2, 2023 05:53
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 13, 2023
@sbc100
Remove dependency on assert from SAFE_HEAP. NFC
e38b58e 
As of emscripten-core#20592, `assert` is no longer defined in release builds but its
still possible to use SAFE_HEAP IN release builds.  Instead of
complicating the definition of `assert` to match `preamble_minimal.js`
this change instead removed the dependency of SAFE_HEAP on the assert
function.

This fixes the `core2s.test_module_wasm_memory` which is currently
failing on the emscripten-releases tree.
@sbc100 sbc100 mentioned this pull request Nov 13, 2023
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 13, 2023
@sbc100
Remove dependency on assert from SAFE_HEAP. NFC
97a7b39 
As of emscripten-core#20592, `assert` is no longer defined in release builds in STRICT
mode, but its still possible to use SAFE_HEAP in this configuration.
Instead of complicating the definition of `assert` to match
`preamble_minimal.js` this change instead removed the dependency of
SAFE_HEAP on the assert function.

This fixes the `core2s.test_module_wasm_memory` which is currently
failing on the emscripten-releases tree.
sbc100 added a commit that referenced this pull request Nov 13, 2023
@sbc100
Remove dependency on assert from SAFE_HEAP (#20696)
457ea77 
As of #20592, `assert` is no longer defined in release builds in STRICT
mode, but its still possible to use SAFE_HEAP in this configuration.
Instead of complicating the definition of `assert` to match
`preamble_minimal.js` this change instead removed the dependency of
SAFE_HEAP on the assert function.

This fixes the `core2s.test_module_wasm_memory` which is currently
failing on the emscripten-releases tree.
@sbc100 sbc100 mentioned this pull request Nov 15, 2023
Closed
@dasa
Copy link
Contributor

dasa commented Jan 19, 2024 •
edited
Loading

This seems to have broken WebIDL bindings when using STRICT mode. It tries to call assert from ensureCache.prepare and ensureCache.alloc.

e.g.

emscripten/tools/webidl_binder.py

Line 207 in edc1a75

assert(ensureCache.buffer);

@dasa dasa mentioned this pull request Jan 23, 2024
Open
sbc100 added a commit to sbc100/emscripten that referenced this pull request Jan 25, 2024
@sbc100
Fix webidl_binder so it can't handle missing assert function
3b6ab55 
This can happen when ASSERTIONS is not set and we are in STRICT
mode or MINIMAL_RUNTIME mode.

See emscripten-core#20592
@sbc100 sbc100 mentioned this pull request Jan 25, 2024
Open
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 25, 2024

This seems to have broken WebIDL bindings when using STRICT mode. It tries to call assert from ensureCache.prepare and ensureCache.alloc.

e.g.

emscripten/tools/webidl_binder.py

Line 207 in edc1a75

assert(ensureCache.buffer);

Thanks @dasa, fix is in #21171

sbc100 added a commit to sbc100/emscripten that referenced this pull request Jan 25, 2024
@sbc100
Fix webidl_binder so it can't handle missing assert function
1c1ada4 
This can happen when ASSERTIONS is not set and we are in STRICT
mode or MINIMAL_RUNTIME mode.

See emscripten-core#20592
@sbc100 sbc100 mentioned this pull request Jan 29, 2024
Open
@cwoffenden cwoffenden mentioned this pull request Feb 23, 2024
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Jan 3, 2025
@sbc100
Fix webidl_binder so it can't handle missing assert function
1348795 
This can happen when ASSERTIONS is not set and we are in STRICT
mode or MINIMAL_RUNTIME mode.

See emscripten-core#20592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbc100 @kripken @dasa